I would really recommend avoiding 2 blockchain wide event filters in one function, this can take quite a while on a real network (especially if using remote endpoints like infura). Instead we could compute this per peer using s.contract.availableBalanceFor(peer) + s.contract.paidOut(p.beneficiary) - p.getLastSentCheque().CumulativeAmount.

If we need this for all peers (not sure why we would though) we can compute liquidBalance() + SUM(s.contract.paidOut(peer)) - SUM(p.getLastSentCheque().CumulativeAmount). The last sum can be computed locally since we have all cheques in our store.

If we implement it that way we also don't need s.paidOut.

Hi Ralph, this call is needed for a node operator to figure out how much balance is still left in his chequebook against which new cheques can be written. It allows him to figure out what his spendable balance is and when he has to re-deposit. Furthermore, this call is needed by Swarm in order to verify that an to-be-send cheque is not overspending.

I will see how and if I can incorporate your proposed formula. Thanks for the suggestion!

Hi Ralph,
Based on your comment, I removed paidOut from the swap structure and place availableBalance there instead. This change minimizes the need to ever call the ComputeAvailableBalance function to one (boot-up).

If we would go for implementing your (second) formula instead of how it is implemented now with the event filters, we need to do a call to the smart-contract to read paidOut for every peer.

Is this indeed quicker than the event filter? Note that Infura recently updated how they handle event queries: https://blog.infura.io/faster-logs-and-events-e43e2fa13773

If you think that what you propose is indeed much quicker, I will make the update!

Note: the option you presented and the one which is currently implemented are not entirely the same. They deviate in the case where a cheque is sent outside of Swarm and already cashed. The current implementation would recognize this (and calculate the available balance correctly from the point of cashing onwards), while your proposal would never recognize this cheque as it is not cashed by a peer, hence the availableBalance will always be off.

We should not consider the case with cheques outside of Swarm, this is always a user error and if done other things will break anyway. If we want the chequebook to be able to be used for other purposes we should introduce an RPC call for that so that the actual cheque creation and sending still takes place in the Swarm node.

Blockchain-wide event queries (from block 0 to "latest") can be really slow in my experience (Have you tested this call against ropsten or mainnet?). The Infura optimisation doesn't help much if it is then still slow for other users. (Since it's only being called once now it's not as big of a problem anymore).

While true that my formula does require more calls to paidOut(), this is an eth_call operation against the latest state which is a very fast operation. My per-peer formula would also have the advantage that it correctly accounts for any set hard deposit.

The current approach also doesn't update availableBalance correctly if the balance increases due to a cashed cheque after the node has started.

Great answer Ralph. I am almost convinced :). Are you proposing to always do the call to availableBalance (not only during start-up), meaning we loop over the peers and do the required eth_call?

I would compute availableBalance on a per-peer basis whenever we want to send a cheque. Then it only requires two eth_call for every cheque sent.

If we want this availableBalance for all peers we have to do an eth_call for every peer but that would only happen when the RPC call is made and not during regular operation.

great discussion guys!

i don't understand the issue as in-depth as you two, but at a high level, it seems sensible to me to query the source of the data (in this case the blockchain) for the latest info when it's needed.

but this, of course, is contingent upon these queries not slowing down the system significantly. it might be hard to verify this.

is the alternative (proposed by @Eknir) slower overall? or is it just less robust/more complicated? i understand the query from the genesis block to the present is slow according to @ralph-pichler, but it would be less queries in the long run, right?

we need availableBalance for all peers in order to know how far away we are from needing to make a new deposit, right?

@mortelli , it's an interesting discussion indeed. At this stage, I would personally favor robustness over efficiency, as we can always make efficiency gains later on, but robustness flaws are probably less nice to detect in a live network.
I think the alternative which I proposed was not necessarily slower, as it would only call to the blockchain upon boot-up. Subsequently, the idea was to keep a shadow view of the state of the blockchain, which requires little resources, but it is definitely less robust than calling the blockchain whenever needed.

If I may put my thoughts here. I pretty much have to agree with @ralph-pichler.

Blockchain-wide event queries (from block 0 to "latest") can be really slow in my experience (Have you tested this call against ropsten or mainnet?). The Infura optimisation doesn't help much if it is then still slow for other users. (Since it's only being called once now it's not as big of a problem anymore).

As @ralph-pichler says these events are horribly slow. We've been using them in Giveth and at one point it took 1.5hs to reprocess everything (around 5000 events). And this is after optimizations like querying since the contract deployment etc. Sure here it will not be that extreme in most of the cases but we should at minimum test it. And yeah chain reorgs or connection issues created a lot of errors and had to be handled.

We should not consider the case with cheques outside of Swarm, this is always a user error and if done other things will break anyway.

I also agree here. At least for the immediate future, I don't think we should be too bothered by this case. We should make an issue about this but not actively solve it now.

One of the usecases I can imagine is when user runs multiple swarm nodes with one chequebook. Still this brings a lot of problems to solve on the accounting side because it introduces all sorts of concurrency issues.

This doesn't need to be changed in this PR but we really need to start thinking about contexts and timeouts when doing blockchain interaction. Here we call PaidOut in a loop, each call might take a while or never terminate at all. Can you open an issue for this in case there is none?

I will

Please comment on the issue @ralph-pichler , with your recommendation for setting context. Issue: #1910

comment exported func

Done!